-
Notifications
You must be signed in to change notification settings - Fork 334
bluez: Verify that Advertisement Monitor has been registered #1140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
bluez: Verify that Advertisement Monitor has been registered #1140
Conversation
bleak/backends/bluezdbus/manager.py
Outdated
| reply = await self._bus.call( | ||
| Message( | ||
| destination=defs.BLUEZ_SERVICE, | ||
| path=adapter_path, | ||
| interface=defs.PROPERTIES_INTERFACE, | ||
| member="Get", | ||
| signature="ss", | ||
| body=[ | ||
| defs.ADVERTISEMENT_MONITOR_MANAGER_INTERFACE, | ||
| "SupportedFeatures", | ||
| ], | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be to execute
reply = await self._bus.call(
Message(
destination=defs.BLUEZ_SERVICE,
path=monitor_path,
interface=defs.PROPERTIES_INTERFACE,
member="Get",
signature="ss",
body=[defs.ADVERTISEMENT_MONITOR_INTERFACE, "Type"],
)
)however this fails with ['Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn\'t exist\n']. Considering that even await asyncio.sleep(0.1) was sufficient here, I left this call which always succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think asyncio.sleep(0) is idiomatic if we just need to yield from the coroutine momentarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, and it is not enough. I think that await asyncio.sleep(0) does yield, but it does not assure that the D-Bus event/operation queue is completely processed, or processed at all. Just for curiosity, asyncio.sleep(0.001) "always" does the job, 0.0001 does the job in about half of the tries, 0.00001 is never enough.
dlech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting idea. Since Release being called could be for other reasons besides this one particular case, maybe it is better to just stick with the os.uname() check and fail early?
bleak/backends/bluezdbus/manager.py
Outdated
| reply = await self._bus.call( | ||
| Message( | ||
| destination=defs.BLUEZ_SERVICE, | ||
| path=adapter_path, | ||
| interface=defs.PROPERTIES_INTERFACE, | ||
| member="Get", | ||
| signature="ss", | ||
| body=[ | ||
| defs.ADVERTISEMENT_MONITOR_MANAGER_INTERFACE, | ||
| "SupportedFeatures", | ||
| ], | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think asyncio.sleep(0) is idiomatic if we just need to yield from the coroutine momentarily.
My first thought - before further analysing the Checking the source https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adv_monitor.c#n520, I see that D-Bus
I like the |
2c4800e to
57a68f9
Compare
|
Done some more testing, using kernel <5.10 and >5.10. In case of kernel <5.10, In case of kernel >5.10, async with BleakScanner(..., scanning_mode="passive"):
await asyncio.sleep(0.0001)no I added example file: |
d952af5 to
a74f1d9
Compare
|
The scope of this PR is creeping. Can we please stick to just one change at a time? Regarding checking if
This seems like it would require a change to BlueZ, such as exposing a list of supported management APIs. Since Release can be called for multiple reasons, it doesn't seem like a reliable solution for detecting support of passive scanning (but it still does seem worth handling the call to Release since we have seen that not handling can lead to waiting for advertisements that will never come). |
a74f1d9 to
eead505
Compare
bleak/backends/bluezdbus/manager.py
Outdated
| # It is important to export AFTER registering, otherwise BlueZ won't use the monitor | ||
| self._bus.export(monitor_path, monitor) | ||
|
|
||
| # During export, MGMT command "Add Advertisement Patterns Monitor (0x0052)" is executed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to detect "org.bluez.AdvertisementMonitorManager1" in the adapter details for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion, but unfortunately this would not solve this particular issue.
The issue here is that not only that org.bluez.AdvertisementMonitorManager1 is present in managed objects, even calling org.bluez.AdvertisementMonitorManager1.RegisterMonitor() succeeds. Only "later" kernel responds with Unknown Command (0x01) to underlying MGMT command Add Advertisement Patterns Monitor (0x0052) - this response is processed by the BlueZ code and registered AdvertisementMonitor1 is destroyed, but the error is not propagated to the D-Bus caller (bleak) in any way.
932d1f5 to
c798a47
Compare
|
Rebased on #1146. |
bleak/backends/bluezdbus/manager.py
Outdated
| # kernel does not support passive scanning and error was returned when trying to execute | ||
| # MGMT command "Add Adv Patterns Monitor" (see https://github.com/hbldh/bleak/issues/1136). | ||
| def advertisement_monitor_released() -> None: | ||
| # FIXME: This is not actually raised |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've again hit a roadblock and would be happy for some ideas on how to solve it. So, there are many reasons why active discovery could be (prematurely) stopped (Discovering set to False), but only a specific case when Advertising Monitor is released.
I want to use this fact to detect that the passive scanning is not supported, instead of just informing the user that something stopped the discovery (e.g. using HCI injection or mgmt commands) and it should be restarted (which would not help in this case).
Trying to achieve that BleakNoPassiveScanError would be raised (preferably in the start() method), I noticed that I came really close to the previous solution, where I "processed the D-Bus events" after exporting the Advertising Manager 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment belongs to older implementation, available here https://github.com/bojanpotocnik/bleak/blob/archive/develop/1136_verify_adding_adv_monitor/bleak/backends/bluezdbus/manager.py#L487
|
Based on the BlueZ tests, it seems like https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/test/example-adv-monitor#n328 # Register the app root path to expose advertisement monitors.
# Release() should get called on monitor0 - incorrect monitor type.
# Activate() should get called on monitor1.
ret = app.register_app()
if not ret:
print('RegisterMonitor failed.')
mainloop.quit()
exit(-1) |
c798a47 to
8223b24
Compare
|
After further checking BlueZ sources and tests, I used the fact that either |
8223b24 to
4e371bf
Compare
bleak/backends/bluezdbus/manager.py
Outdated
| # If advertisement monitor is released before the scanning is stopped, it means that the | ||
| # kernel does not support passive scanning and error was returned when trying to execute | ||
| # MGMT command "Add Adv Patterns Monitor" (see https://github.com/hbldh/bleak/issues/1136). | ||
| # Otherwise, monitor will be activated and start to receive advertisement packets. | ||
| monitor_processed = asyncio.Queue() | ||
|
|
||
| try: | ||
| monitor = AdvertisementMonitor(filters, discovery_stopped_callback) | ||
| monitor = AdvertisementMonitor(filters, monitor_processed.put_nowait) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also keep the discovery_stopped_callback functionality, if required, by adding an intermediate callback which will call put_nowait and then discovery_stopped_callback.
cbdfdc8 to
db0be8c
Compare
|
As the latest solution does not really use features from #1146, except adding a comment explaining why callback is not added to BlueZManager -> async passive_scan() -> async with self._bus_lock:
self._device_removed_callbacks.append(device_removed_callback_and_state)
# Once a monitoring job is activated by BlueZ, the client can expect to get notified
# on the targeted advertisements no matter if there is an ongoing discovery session
# (started/stopped with org.bluez.Adapter1.StartDiscovery/StopDiscovery, as done when
# using active scanning).
# That is why discovery_stopped_callback is not added to self._discovery_stopped_callbacks
# at this point, as org.bluez.Adapter1.Discovering status is not relevant for passive
# scanning.
# If advertisement monitor is released before the scanning is stopped, it means that theI've moved it to bojanpotocnik:1136_verify_adding_adv_monitor_rebased_1146 and rebased this one on develop. |
This exception can be used to e.g. retry scanning with scan_mode adjusted to "active" on systems not supporting the "passive" scan.
In case of BlueZ, using passive scanning mode is not simply passing "passive" as a ``scan_mode`` parameter, but requires more arguments. This example showcases that.
db0be8c to
aaa5788
Compare
Solves #1136